Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor changes for Scala.js 1.0 compatibility #3246

Merged
merged 2 commits into from
Jan 18, 2020

Conversation

travisbrown
Copy link
Contributor

@travisbrown travisbrown commented Jan 9, 2020

These changes make it possible to build Cats on Scala.js 1.0 (currently 1.0.0-RC2) via an environmental variable, without actually changing anything significant about the current build, which is still on Scala.js 0.6.

This approach is currently used by Simulacrum, Discipline (although Discipline does test Scala.js 1.0.0-RC2 in CI, and this change doesn't), etc.

I've confirmed that the following succeeds:

SCALAJS_VERSION=1.0.0-RC2 sbt +validateJS

class FutureTests extends CatsSuite {
// Replaces Scala.js's `JSExecutionContext.runNow`, which is removed in 1.0.
// TODO: We shouldn't do this! See: https://github.com/scala-js/scala-js/issues/2102
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably worth creating and linking to a github issue about fixing this item

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: #3247

@codecov-io
Copy link

codecov-io commented Jan 9, 2020

Codecov Report

Merging #3246 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3246   +/-   ##
=======================================
  Coverage   93.05%   93.05%           
=======================================
  Files         376      376           
  Lines        7413     7413           
  Branches      201      201           
=======================================
  Hits         6898     6898           
  Misses        515      515
Flag Coverage Δ
#scala_version_212 93.38% <ø> (+0.02%) ⬆️
#scala_version_213 92.82% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2f8a96...eeecbc3. Read the comment docs.

@djspiewak
Copy link
Member

This effectively means that sbt release will not work anymore, since the release needs to be split across two separate sbt runs. I don't know another way to do this, but it also means that, in order to keep the release documented and reproducible, we should toss a shell script in here somewhere which replaces the functionality we're currently getting from sbt-release while juggling the environment variables.

@travisbrown
Copy link
Contributor Author

@djspiewak Sure, I'll add a script. My preference would be to keep this fairly low-key, though—I don't want to commit to cross-building in the long term, or to make CI builds even longer by testing 1.0.0-RC2 on every PR now.

@dwijnand
Copy link
Contributor

dwijnand commented Jan 9, 2020

Perhaps you could switch to a system property, and do something like this:

releaseProcess := Seq[ReleaseStep](
  ...
  publishArtifacts,
  releaseStepCommand("""eval sys.prop("scalajs.version") = "1.0.0-RC2"; reload"),
  publishArtifacts,
  ...
)

@travisbrown
Copy link
Contributor Author

@dwijnand Noooooo, please, no. Every time I've ever done anything like that in any build I've regretted it. All of this stuff is already a fragile pile of crap on the edge of disaster. I want to unblock people who want 1.0.0 primarily because I want us to be in a position to drop 0.6 the minute 1.0.0 final is out, and that's unlikely to be more than one or two Cats releases away. I think we should stick to the absolute minimum amount of extra process.

LukaJCB
LukaJCB previously approved these changes Jan 9, 2020
@dwijnand
Copy link
Contributor

dwijnand commented Jan 9, 2020

WFM 😄

@travisbrown
Copy link
Contributor Author

@dwijnand In any case that would publish the JVM artifacts twice.

@dwijnand
Copy link
Contributor

dwijnand commented Jan 9, 2020

Yeah, but I think that's fine if you're using the new sonatype bundle thing.

@travisbrown
Copy link
Contributor Author

And here's the 2.1.0 backport branch, which also passes SCALAJS_VERSION=1.0.0-RC2 sbt +validateJS: https://github.com/typelevel/cats/tree/backport/scala.js-1.0/2.1.0

@travisbrown
Copy link
Contributor Author

@djspiewak Any objection to merging now?

@cquiroz
Copy link
Contributor

cquiroz commented Jan 15, 2020

@travisbrown Thanks for this. it would help unlock a big part of the ecosystem that can't be ported to RC2 just yet

@travisbrown travisbrown merged commit 95cb4ae into typelevel:master Jan 18, 2020
@travisbrown travisbrown added this to the 2.2.0-M1 milestone Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants